fix(server): Sequence destroy() after native teardown - #1513
Open
RandomByte wants to merge 1 commit into
Open
Conversation
RandomByte
marked this pull request as ready for review
August 7, 2026 09:57
matz3
requested changes
Aug 11, 2026
RandomByte
force-pushed
the
fix/server-destroy-teardown-race
branch
2 times, most recently
from
August 11, 2026 09:41
51f1c93 to
480feaf
Compare
matz3
reviewed
Aug 11, 2026
matz3
left a comment
matz3
left a comment
Member
There was a problem hiding this comment.
LGTM, but the commit message / PR description of the "Fix" need to be updated, as they still describe the firing of the callback
Supervisor.destroy() passed the caller's callback to http.Server.close(), firing it on socket close. The BuildServer's teardown (the @parcel/watcher unsubscribe and the node:sqlite handle, which maps a 256 MB region of the database into memory) runs after that call and was not sequenced ahead of the callback, so server.close(resolve) resolved with SQLite still mid-close. The next test.serial opening a fresh DatabaseSync, or the worker exiting, then races the finalizing handle and raises an access violation on Windows (0xC0000005), surfacing as `exited with a non-zero exit code: 3221225477` in the reinitialize suite. Make destroy() a plain promise: start the socket close, then await the definition watcher and BuildServer teardown, and await the socket last. The promise resolves only once every native handle is closed. server.js adapts this to the existing close(callback) API and also returns the promise when no callback is passed, so callers can await it; a teardown error is passed to the callback (and rejects the promise) instead of being swallowed.
RandomByte
changed the title
RandomByte
force-pushed
the
fix/server-destroy-teardown-race
branch
from
August 11, 2026 15:48
480feaf to
924f9df
Compare
Member
Author
Thanks, I missed that. Updated the commit message and PR body |
matz3
approved these changes
Aug 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes a unit test failing flakily in CI, most often on Node 22/24 on Windows, with
test/lib/server/reinitialize.js exited with a non-zero exit code: 3221225477. That code is0xC0000005, a native access violation on process teardown, not a test-assertion failure. The defect is in production teardown code, so the fix lands inSupervisor.js; the flaky reinitialize test is what surfaced it.Root cause
Supervisor.destroy() passed the caller's callback to http.Server.close(), firing it on socket close. The BuildServer's teardown (the
@parcel/watcherunsubscribe and the node:sqlite handle, which maps a 256 MB region of the database into memory) runs after that call and was not sequenced ahead of the callback, so server.close(resolve) resolved with SQLite still mid-close. The next test.serial opening a fresh DatabaseSync, or the worker exiting, then races the finalizing handle and raises an access violation on Windows (0xC0000005), surfacing asexited with a non-zero exit code: 3221225477in the reinitialize suite.Fix
Make destroy() a plain promise: start the socket close, then await the definition watcher and BuildServer teardown, and await the socket last. The promise resolves only once every native handle is closed. server.js adapts this to the existing close(callback) API and also returns the promise when no callback is passed, so callers can await it; a teardown error is passed to the callback (and rejects the promise) instead of being swallowed.